fix: memory & lock issues in sortTransactions #588
Conversation
WalkthroughThe changes remove the historical balance tracking from the DSAccount class. In particular, the method for obtaining the balance after a transaction has been removed from both the header and implementation files. Additionally, the balanceHistory property and all associated logic have been eliminated, and the transaction sorting logic has been updated to cache address values locally. Changes
Sequence Diagram(s)sequenceDiagram
participant Tx as Transaction
participant Acc as DSAccount
Tx ->> Acc: Request balance update
Acc ->> Acc: Process transaction (removed history tracking)
Acc ->> Acc: Cache external/internal addresses for sorting
Acc -->> Tx: Return updated balance
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -289,9 +289,6 @@ FOUNDATION_EXPORT NSString *_Nonnull const DSAccountNewAccountShouldBeAddedFromT | |||
| // returns the fee for the given transaction if all its inputs are from wallet transactions, UINT64_MAX otherwise | |||
| - (uint64_t)feeForTransaction:(DSTransaction *)transaction; | |||
|
|
|||
| // historical wallet balance after the given transaction, or current balance if transaction is not registered in wallet | |||
| - (uint64_t)balanceAfterTransaction:(DSTransaction *)transaction; | |||
| return NO; | ||
| }; | ||
|
|
||
| NSArray *externalAddresses = self.externalAddresses; | ||
| NSArray *internalAddresses = self.internalAddresses; |
There was a problem hiding this comment.
self.externalAddresses and self.internalAddresses was calculated from scratch on every tx comparison
| @@ -895,21 +879,22 @@ __block __weak BOOL (^_isAscending)(id, id) = isAscending = ^BOOL(DSTransaction | |||
| }] != NSNotFound) return NO; | |||
| if ([self.invalidTransactionHashes containsObject:hash1] && ![self.invalidTransactionHashes containsObject:hash2]) return YES; | |||
| if ([self.pendingTransactionHashes containsObject:hash1] && ![self.pendingTransactionHashes containsObject:hash2]) return YES; | |||
| for (DSTransactionInput *input in tx1.inputs) { | |||
| if (_isAscending(self.allTx[uint256_obj(input.inputHash)], tx2)) return YES; | |||
| } | |||
There was a problem hiding this comment.
Precise sorting causes long waits for the lock to be freed, which leads to stalled sync and UI freezes occasionally.
It doesn't look like we need precise sorting anywhere.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
DashSync/shared/Models/Wallet/DSAccount.m (2)
882-882: Consider adding a deterministic fallback for consistent ordering.Currently, if none of the conditions match, this final
return NO;might produce an ambiguous ordering. Consider adding a final comparison step (e.g., comparing transaction hashes) to guarantee stable and deterministic sorting.- return NO; + // Fallback tiebreak: compare tx hashes + NSComparisonResult hashComparison = memcmp(tx1.txHash.u8, tx2.txHash.u8, sizeof(tx1.txHash)); + if (hashComparison < 0) return YES; + if (hashComparison > 0) return NO; + return NO; // remain consistent if truly identical
894-895: Clarify the sorting logic when checking internal vs. external addresses.It's not immediately clear why we look up
internalAddressesfirst fortx1and partially rely onexternalAddressesfortx2. A short explanatory comment or small extract method would improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
DashSync/shared/Models/Wallet/DSAccount.h(0 hunks)DashSync/shared/Models/Wallet/DSAccount.m(1 hunks)
💤 Files with no reviewable changes (1)
- DashSync/shared/Models/Wallet/DSAccount.h
🔇 Additional comments (3)
DashSync/shared/Models/Wallet/DSAccount.m (3)
886-887: Caching addresses in local variables is a good performance measure.Referencing
externalAddressesandinternalAddressesin local arrays avoids repeated property lookups in the sorting logic. This also minimizes lock durations, which helps performance.
897-897: Verify symmetrical logic for both transactions.The code adjusts
iif it was not found in internal addresses, but there's no corresponding check forj. This could cause an asymmetrical ordering iftx2has no internal addresses. Consider ensuring both transactions are handled consistently.
899-899: Confirm reversed numeric comparison is intentional.The returned result inverts the usual ordering if
i > jis treated as ascending. Confirm that this reversed logic suits the domain requirement. If you prefer smaller indices to sort first, swap the return values.
Issue being fixed or feature implemented
sortTransactionsmethod causes memory spikes on start and deadlocks during syncWhat was done?
sortTransactionssortTransactionsto avoid long locksHow Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit